Rename Authentication, User to Controller, Model #268

croaky · 2013-02-24T06:13:53Z

No description provided.

gylaz · 2013-02-25T05:44:52Z

lib/clearance/model.rb

@@ -10,8 +10,10 @@ module User

      include Validations
      include Callbacks
-      include (Clearance.configuration.password_strategy ||
-        Clearance::PasswordStrategies::BCrypt)
+      include (


Space before (

croaky · 2013-02-25T06:13:22Z

Thanks, @gylaz. Applied your feedback in 9b1cba9.

jferris · 2013-02-25T16:47:54Z

I like the information the old names provided. I think the main issue was that Clearance::Authentication maybe misrepresented what it provided to controllers, but I think the new names go too far in the other direction.

Clearance itself has multiple controllers, and they don't descend from Clearance::Controller, which I find a little suprising. The Controller name tells you what it's meant to be mixed into, but not what it provides. Maybe AuthenticatedController or something like that? The module provides helpers related to authentication (with just a sprinkle of authorization) and is meant to be mixed into ApplicationController.

I think Clearance::User was fine as a name. Was there a specific issue that renaming was designed to fix? The new name doesn't tell you what kind of role the model plays. "Model" basically means "Thing" and has some vague connotations when it's in a Rails application, whereas "User" means something pretty specific in a Rails application.

If we do rename classes from the public API, do we want to include deprecated fallbacks under the old names? I know clearance is technically not 1.0 yet, but there are a lot of applications out there that will break if we rename these.

croaky · 2013-02-25T16:53:44Z

I think the main issue was that Clearance::Authentication maybe misrepresented what it provided to controllers, but I think the new names go too far in the other direction.

Right, this was the issue I was trying to address. Also, it's not the entire contents of the Clearance::Authentication that are potentially misrepresented, just authorize in the public API (the private API of store_location, return_to, and clear_return_to might be a worthwhile different conversation).

I don't feel strongly about this change. I wanted to make the change to see what it looked and felt like and am seeking feedback like yours about whether this causes more trouble than it's worth. I also wanted to do it pre-1.0, which we're only one issue or two away from reaching: #243

jferris · 2013-02-26T16:26:43Z

My general feelings about the authorize vs authenticate debate:

The authorize method performs authorization - it denies access to unauthenticated users. The sessions controller performs authentication, as does Clearance::Session.
I've generally assumed that controllers would override authorize for controllers that require specific authentication.
It's sort of strange that Clearance::Authentication contains a bunch of authorization logic.

How about this?

Revert Clearance::Model back to Clearance::User.
Split Clearance::Controller into Clearance::Authentication and Clearance::Authorization, both of which get mixed into Clearance::Controller.
Mix Clearance::Controller into ApplicationController in the install generator.

In order to clarify the authorize vs authenticate debate, we can define the method as authorize_authenticated_users and then just call that from authorize. Not sure if that actually helps, though.

croaky · 2013-03-02T18:45:02Z

@jferris Done. Ready for re-review.

jonathanhefner · 2013-03-03T22:18:25Z

To add a use case to the authorize vs authenticate method debate:

rescue_from CanCan::AccessDenied do |exception|
  if signed_in?
    raise
  else
    # get the user to sign in, since they haven't already done so
    authorize
  end
end

To me, the call to authorize makes a lot more sense as authenticate because all you're asking the user to do is verify they are who they say they are by providing an email/password. It isn't authorizing them for anything--even if they complete the authorize successfully (i.e. successfully log in), they can still be denied access.

But in the end, it's just one line of code in one base controller, and a comment right next to it suffices to resolve any confusion down the road.

croaky · 2013-03-03T22:26:55Z

@jonathanhefner In that use case, would redirect_to sign_in work better in your else branch?

authorize calls unless signed_in?, which you already cover. It then calls deny_access, which stores the current location then calls redirect_to url_after_denied_access_when_signed_out.

Alternatively, could you use deny_access or redirect_to url_after_denied_access_when_signed_out in the constraint?

calebhearth · 2013-03-04T03:59:09Z

lib/clearance/authentication.rb

@@ -4,8 +4,14 @@ module Authentication

    included do
      helper_method :current_user, :signed_in?, :signed_out?
-      hide_action :authorize, :current_user, :current_user=, :deny_access,
-        :sign_in, :sign_out, :signed_in?, :signed_out?
+      hide_action(


If we just make these methods private, they don't need to be hidden.

@calebthompson If we make them private methods, we get errors like the following:

private method `signed_in?' called for #<ForgeriesController:0x007f8338c2dc48> private method `current_user=' called for #<Clearance::SessionsController:0x007f833c8bd848>

Ah, that makes sense. Why aren't these in a helper module?

On Sun, Mar 3, 2013 at 7:31 PM, Dan Croak notifications@github.com
wrote:

@@ -4,8 +4,14 @@ module Authentication

included do helper_method :current_user, :signed_in?, :signed_out?

hide_action :authorize, :current_user, :current_user=, :deny_access,

:sign_in, :sign_out, :signed_in?, :signed_out?

hide_action(
@calebthompson If we make them private methods, we get errors like the following:

private method `signed_in?' called for #<ForgeriesController:0x007f8338c2dc48> private method `current_user=' called for #<Clearance::SessionsController:0x007f833c8bd848>

Reply to this email directly or view it on GitHub:
https://github.com/thoughtbot/clearance/pull/268/files#r3219887

I'm not sure I know what you mean by a helper module. Are you referring to the helper method?

http://api.rubyonrails.org/classes/AbstractController/Helpers/ClassMethods.html#method-i-helper

If the goal is to make current_user, signed_in?, and signed_out? available to view templates, that's handled by line 6. The other methods are intended to be used by controllers.

Ah, that's clear now. Never mind, I should have paid more attention to what the hidden actions were.

On Sun, Mar 3, 2013 at 8:04 PM, Dan Croak notifications@github.com
wrote:

@@ -4,8 +4,14 @@ module Authentication

included do helper_method :current_user, :signed_in?, :signed_out?

hide_action :authorize, :current_user, :current_user=, :deny_access,

:sign_in, :sign_out, :signed_in?, :signed_out?

hide_action(

I'm not sure I know what you mean by a helper module. Are you referring to the helper method?
http://api.rubyonrails.org/classes/AbstractController/Helpers/ClassMethods.html#method-i-helper
If the goal is to make current_user, signed_in?, and signed_out? available to view templates, that's handled by line 6. The other methods are intended to be used by controllers.
Reply to this email directly or view it on GitHub:
https://github.com/thoughtbot/clearance/pull/268/files#r3219972

Okay, cool. No worries. Thanks for the comments. Got me wondering whether this stuff was still necessary on current versions of Rails and refreshed my memory on how hide_action and module visibility worked.

jonathanhefner · 2013-03-05T17:21:35Z

@croaky Looking at the implementation of authorize and deny_access, I could simply call deny_access directly (because I don't need the extra signed_in? check, as you pointed out). The effect would be the same though: I'd still add a comment to that method call to note that I'm only getting to the user to sign in, not denying (or authorizing) them for access. (Semantics, I know. :)

croaky · 2013-03-11T01:40:14Z

@jferris Could I get you to re-review this one when you have a moment?

jferris · 2013-03-11T14:52:13Z

This looks good to me.

croaky · 2013-03-11T16:08:34Z

Thanks, @jferris. Merged as 91a984c.

There has been confusion about the `authorize` method residing in the `Authentication` module: * The `authorize` method performs authorization - it denies access to unauthenticated users. * It is assumed that controllers would override `authorize` for controllers that require specific authentication. * It's sort of strange that `Clearance::Authentication` contains a bunch of authorization logic. So, we: * Split `Clearance::Controller` into `Clearance::Authentication` and `Clearance::Authorization`, both of which get mixed into `Clearance::Controller`. * Mix `Clearance::Controller` into `ApplicationController` in the install generator. Read more: thoughtbot#268 thoughtbot#257

croaky mentioned this pull request Feb 24, 2013

Move authorize method out of Authentication module #257

Closed

gylaz reviewed Feb 25, 2013
View reviewed changes

Dan Croak added 4 commits March 2, 2013 09:21

Rename Authentication, User to Controller, Model

33e2e7e

Apply Greg's feedback from review

7c3bffd

Revert to Clearance::User

c536948

Split Clearance::{Authentication,Authorization}

2ae5a1a

croaky mentioned this pull request Mar 2, 2013

Authenticate vs Authorize #239

Closed

calebhearth reviewed Mar 4, 2013
View reviewed changes

croaky closed this Mar 11, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename Authentication, User to Controller, Model #268

Rename Authentication, User to Controller, Model #268

croaky commented Feb 24, 2013

gylaz Feb 25, 2013

croaky commented Feb 25, 2013

jferris commented Feb 25, 2013

croaky commented Feb 25, 2013

jferris commented Feb 26, 2013

croaky commented Mar 2, 2013

jonathanhefner commented Mar 3, 2013

croaky commented Mar 3, 2013

calebhearth Mar 4, 2013

croaky Mar 4, 2013

calebhearth Mar 4, 2013

croaky Mar 4, 2013

calebhearth Mar 4, 2013

croaky Mar 4, 2013

jonathanhefner commented Mar 5, 2013

croaky commented Mar 11, 2013

jferris commented Mar 11, 2013

croaky commented Mar 11, 2013

Rename Authentication, User to Controller, Model #268

Rename Authentication, User to Controller, Model #268

Conversation

croaky commented Feb 24, 2013

gylaz Feb 25, 2013

Choose a reason for hiding this comment

croaky commented Feb 25, 2013

jferris commented Feb 25, 2013

croaky commented Feb 25, 2013

jferris commented Feb 26, 2013

croaky commented Mar 2, 2013

jonathanhefner commented Mar 3, 2013

croaky commented Mar 3, 2013

calebhearth Mar 4, 2013

Choose a reason for hiding this comment

croaky Mar 4, 2013

Choose a reason for hiding this comment

calebhearth Mar 4, 2013

Choose a reason for hiding this comment

croaky Mar 4, 2013

Choose a reason for hiding this comment

calebhearth Mar 4, 2013

Choose a reason for hiding this comment

croaky Mar 4, 2013

Choose a reason for hiding this comment

jonathanhefner commented Mar 5, 2013

croaky commented Mar 11, 2013

jferris commented Mar 11, 2013

croaky commented Mar 11, 2013